Skip to content

Conversation

FrancescaWatson
Copy link
Contributor

… when starting in octave.

@FrancescaWatson FrancescaWatson requested review from moyner and bska May 26, 2025 14:12
Copy link
Contributor

@bska bska left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent–this certainly improves usability of the package. Just a couple of suggestions since we're in this area anyway.

% Add modules as root directories
dirs = {'autodiff', 'model-io', 'multiscale', 'modules', 'solvers', ...
'visualization','co2lab'};
base_path = fileparts(mfilename('fullpath')); %#ok
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I'm reading it incorrectly, this is just rootdir(), i.e., d.

dirs = {'autodiff', 'model-io', 'multiscale', 'modules', 'solvers', ...
'visualization','co2lab'};
base_path = fileparts(mfilename('fullpath')); %#ok
clear fn
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we use/assign fn anywhere, so we could just remove this statement I think.

Comment on lines +90 to +92
for i = 1:numel(dirs)
mrstPath('addroot', fullfile(base_path, dirs{i}));
end
Copy link
Contributor

@bska bska May 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need the index i here. We might as well just write this as

for rdir = dirs
   mrstPath('addroot', fullfile(d, rdir{1}));
end

instead.

If we wish to be very careful/future proof we might even consider writing

for rdir = reshape(dirs, 1, [])
   mrstPath('addroot', fullfile(d, rdir{1}));
end

although that is getting close to going overboard.

Comment on lines +36 to +38
% Automatically load selected modules for backwards compatibility.
autoload = {};
load_compat_modules(autoload);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The set of compatibility modules used to be the "incomp"/"mimetic" modules at some point, but by now it's been empty for a long time. I think we should use the opportunity when fusing these startup functions into one to just remove the compatibility modules altogether.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants